-
Notifications
You must be signed in to change notification settings - Fork 929
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve getting started experience #7011
Conversation
28bcf75
to
a635339
Compare
Reduced scope to just awaiting TLS now that other changes have been implemented. Changed the rollout text a bit since we may to continue waiting for TLS. Also, I removed a "..." because it seems redundant/passive aggressive along with a spinner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some nits
src/apphosting/index.ts
Outdated
@@ -121,6 +148,7 @@ export async function doSetup( | |||
}); | |||
|
|||
await setDefaultTrafficPolicy(projectId, location, backendId, branch); | |||
logSuccess(`Successfully created backend:\n\t${backend.name}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove, this is already on L138
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ugh. merge issues. I had previously moved and now it got turned into an add. TY for the catch.
src/apphosting/index.ts
Outdated
createRolloutSpinner.succeed(`Your backend is now deployed at:\n\thttps://${backend.uri}`); | ||
createRolloutSpinner.succeed("Rollout complete"); | ||
if (!(await tlsReady(url))) { | ||
const tlsSpinner = ora("Waiting for TLS certificate"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you probably want tlsSpinner = ora(...).start()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops! Maybe Ora isn't as easy to use as I thought!
src/apphosting/index.ts
Outdated
const createRolloutSpinner = ora( | ||
"Starting a new rollout... This make take a few minutes. It's safe to exit now.", | ||
"Starting a new rollout; this make take a few minutes. It's safe to exit now.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: this may take
src/apphosting/index.ts
Outdated
createRolloutSpinner.succeed(`Your backend is now deployed at:\n\thttps://${backend.uri}`); | ||
createRolloutSpinner.succeed("Rollout complete"); | ||
if (!(await tlsReady(url))) { | ||
const tlsSpinner = ora("Waiting for TLS certificate"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Finalizing your backend's TLS certificate; this may take a few minutes."
Not sure about some of the prose I've moved about. Feel encouraged to nitpick and/or add other nitpickers as reviewers.